Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop Ruby < 2.7 and deal with kwargs #44

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Aug 31, 2022

Ruby 3 has started to separate args and kwargs.

@ekohl
Copy link
Member Author

ekohl commented Aug 31, 2022

This fails on Ruby < 2.7. We can either use some complicated method to support both, or move forward and drop it. Today Foreman is the main consumer and that only cares about Ruby 2.7+. 2.6 and older are EOL anyway.

@ekohl ekohl changed the title Deal with kwargs Drop Ruby < 2.7 and deal with kwargs Aug 31, 2022
Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this with Foreman and it shows there's some change in the behavior. The kickstart template no longer works, complaining about kickstart_ifcfg_get_identifier_names snippet calling inheriting_mac on nil. This method is called on @interface which is supposed to be a variable holding the interface object. Reading the code, it looks like we can no longer pass variables through the snippet macro.

It seems like the Safemode::Box#eval no longer supports second argument of allowed variables (assigns). While this PR does not touch the code, I guess the kwargs somehow impacts that. However this code seems to work fine

require 'safemode'
require 'erb'

erb_code = %q{<%= @a %>}
box = Safemode::Box.new

puts "Doing the ERB code in safe mode\n-----"
puts box.eval(ERB.new(erb_code).src, { :a => 'b' })

I couldn't find the actual cause, but it's quite simple to reproduce with Foreman. Just create a snippet with <%= @a %> and then call it from the template <%= snippet('name', variables: { :a => 'b' }) %>

if @locals.has_key?(method)
@locals[method]
elsif @delegate_methods.include?(method)
# TODO: deal with kwargs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still a TODO? should we keep it or resolve it before the merge?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this is precisely why it fails. I struggled to find ways to properly test this. Is it me or is there no test coverage for Safemode::Scope?

@ares
Copy link
Member

ares commented Oct 4, 2022

And I can confirm, it's the last commit that caused the change in the behavior.

@ekohl
Copy link
Member Author

ekohl commented Oct 4, 2022

I've tested this with Foreman and it shows there's some change in the behavior.

Since there is a difference in Ruby versions, did you test it with Ruby 2.7 or 3.0?

And I can confirm, it's the last commit that caused the change in the behavior.

Any thoughts on already merging the first 3 commits?

@ares
Copy link
Member

ares commented Oct 5, 2022

It's 2.7.6 available on CentOS Stream 8.

Happy to merge the first three commits if you open them as a separate PR

@ekohl
Copy link
Member Author

ekohl commented Oct 7, 2022

I've pushed a fix that locally made Foreman's rake test TEST=test/controllers/api/v2/report_templates_controller_test.rb:229 pass. However, I'm still running more tests and I also see undefined method '#name' for NilClass::Jail.

@ekohl ekohl marked this pull request as draft October 7, 2022 13:42
Comment on lines 29 to 30
# TODO: this is wrong for Ruby 3 and raises a deprecation warning in 2.7
assert_jailed "@article.to_jail.method_with_kwargs({ :a_keyword => true })", "@article.method_with_kwargs(a_keyword: true)"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tracked it down to this bit. Reading seattlerb/ruby2ruby#55 (comment) I'm afraid we may have to dive into ruby2ruby to properly deal with this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note that ruby2ruby 2.5.0 does add support for kwargs, so we can at least do it correctly.

@ekohl ekohl force-pushed the ruby-3 branch 2 times, most recently from 6c46db6 to adbc3c4 Compare October 13, 2022 14:03

def test_call_with_complex_args
unsafe = "@article.method_with_kwargs('positional', a_keyword: true, **kwargs)"
jailed = "@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got further by copying the implementation from ruby2ruby. However, I don't think **to_jail.kwargs is correct.

@adamruzicka any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That indeed looks awkward..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I found why is that, or at least how to make it be less awkward. It seems like parser needs more context, so if we slightly changes this example to:

unsafe = "kwargs = {a: 1, b: 2} ; @article&.method_with_kwargs('positional', a_keyword: true, **kwargs)"

then the results will change as well:

"kwargs = { :a => 1, :b => 2 }\n@article&.to_jail&.method_with_kwargs(\"positional\", :a_keyword => true, **kwargs)\n"

@ekohl
Copy link
Member Author

ekohl commented Oct 13, 2022

I made some progress by copying ruby2ruby code. While I was going through it I also noticed safe mode was rather trivial so this now also fixes #40.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... As of now it looks sane. I've locally run Foreman tests with this PR and they are green (Ruby 2.7.6).

Although, I'd prefer to extract safe call operator from this PR (I know there is one already) and merge that first.

I'm going to run the tests on Ruby 3 and I'll try to do some testing by hands. If that passes as well, I'd be OK with merging even some tests look awkward :)

I'd highly appreciate some @adamruzicka's eyes on this as well.


def test_call_with_complex_args
unsafe = "@article.method_with_kwargs('positional', a_keyword: true, **kwargs)"
jailed = "@article.to_jail.method_with_kwargs(\"positional\", :a_keyword => true, **to_jail.kwargs)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That indeed looks awkward..

@ofedoren
Copy link
Member

Update: I've run the set of Foreman's tests on Ruby 3.0.4 and with theforeman/foreman#9871 it was green. At least on my machine. I've tried to run GHA with that (ofedoren/foreman#9), but I forgot that this lib is still pinned to Ruby < 3.0.

@ekohl, since we're trying to make this lib compatible with Ruby 3, could we update the pinned version? It'll help with automated tests. Which reminds me, maybe we could update ci.yml here to run the tests on Ruby 3.0?

Looking into manual testing now.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ares, I've re-checked your examples, both work with the latest version of the PR. Anything else comes to your mind worth checking?

Otherwise ACK from me. Still not sure what to do with https://github.com/theforeman/safemode/pull/44/files#diff-5ef4e011cefc0a9aee550ce6870c2739cdb54393cbf42028389a276b4a035aaaR42 though...

This brings it closer to the original, making it easier to see what's
going on. Looking at the git log of ruby2ruby there were no
modifications in this area. The only difference in process_call is that
process_call_receiver is jailed. This implementation actually supports
kwargs and kwsplat.

It also adds tests for kwargs and kwsplat.
Ruby 3 has started to separate args and kwargs.
@ofedoren ofedoren marked this pull request as ready for review December 11, 2023 12:58
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, all, this seems to be ready. I've also included Ruby 3.0 CI and the tests are green. Let's get this in.

@ofedoren ofedoren merged commit eb143d8 into theforeman:master Dec 11, 2023
3 checks passed
@ekohl ekohl deleted the ruby-3 branch December 18, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants